Skip to content

[testing ci] add tests for unicode tarballs, fix utf-8 on macOS CI#20060

Closed
strega-nil wants to merge 3 commits intomicrosoft:masterfrom
strega-nil:utf8-tarball
Closed

[testing ci] add tests for unicode tarballs, fix utf-8 on macOS CI#20060
strega-nil wants to merge 3 commits intomicrosoft:masterfrom
strega-nil:utf8-tarball

Conversation

@strega-nil
Copy link
Contributor

see the errors in #19963

@BillyONeal
Copy link
Member

I think this belongs as an e2e test rather than here?

@strega-nil-ms
Copy link
Contributor

@BillyONeal it's testing vcpkg_extract_source_archive, so having it as a port test is fine, no?

@BillyONeal
Copy link
Member

@BillyONeal it's testing vcpkg_extract_source_archive, so having it as a port test is fine, no?

Kinda related to #19034 where I made the same comment. As far as I know this is testing scripts/cmake meaning it logically belongs in the tool repo.

@strega-nil-ms
Copy link
Contributor

@BillyONeal At that point, we should probably pull out the test_ports directory, but until then the scripts still live in vcpkg, right?

@NancyLi1013 NancyLi1013 added category:scripts-audit category:infrastructure Pertaining to the CI/Testing infrastrucutre labels Sep 9, 2021
@dg0yt
Copy link
Contributor

dg0yt commented Sep 9, 2021

At that point, we should probably pull out the test_ports directory, but until then the scripts still live in vcpkg, right?

Note the table I posted in #19034 (comment). IMO key question is what triggers the test. If scripts are moved out of vcpkg, #20060 can be moved out, too. But some tests in test_ports are really meant to be triggered by changes to ports (i.e. to vcpkg repo), in order to ensure usability of the ports or selected non-default feature combinations.

@strega-nil-ms strega-nil-ms changed the title [testing] add tests for unicode tarballs [testing ci] add tests for unicode tarballs, fix utf-8 on macOS CI Sep 10, 2021
clean: resources
timeoutInMinutes: 1440 # 1 day
variables:
- name: LC_ALL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like maybe vcpkg itself should be setting this rather than hiding it in CI? It would be in keeping with other environment variable handling we do....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to set this variable in general; I don't even know if it's possible. Perhaps we could do some locale parsing; however, for CI, it makes sense that we set it to en_US.UTF-8.

@BillyONeal
Copy link
Member

I think the correct fix here is to have vcpkg set the locale by testing a few common ones like:

  • C.UTF-8
  • POSIX.UTF-8
  • en_US.UTF-8

and choosing the first matching one.

Notably, this change plus #19963 will break existing macos customers, most of whom aren't passing LC_ALL like this.

@JackBoosY
Copy link
Contributor

So, should we wait for the next vcpkg release?

@BillyONeal
Copy link
Member

So, should we wait for the next vcpkg release?

It looks like the current tool should have the fix:

image

@strega-nil-ms
Copy link
Contributor

Closing in favor of the next tool

@strega-nil strega-nil deleted the utf8-tarball branch March 31, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:infrastructure Pertaining to the CI/Testing infrastrucutre

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants